[POC] Prototyping - MySQL User Defined Types - #707
Conversation
Can now add complex numbers.
There was a problem hiding this comment.
Codex PR review
Overall: patch is incorrect (99% confidence)
The change introduces server-crash paths, stale callbacks after component unload, missing authorization and replication handling, unbounded runtime leaks, and discards the type definition itself.
7 finding(s) were posted inline.
| #ifdef WITH_EXPERIMENTAL_UDT | ||
| WARN_NOT_IMPLEMENTED(thd, "Sql_cmd_create_type::execute()"); | ||
|
|
||
| const char *db_name = m_type_ident->db.str; |
There was a problem hiding this comment.
P1: [P1] Resolve unqualified type names before using the schema
The grammar accepts CREATE TYPE foo ..., but Type_ident leaves db.str null for that form. This null value is subsequently passed through schema locking, MDL initialization, and dd::String_type, which can crash instead of using the current database. Resolve an empty schema from THD::db() and report ER_NO_DB_ERROR when none is selected.
Confidence: 99%
| return true; | ||
| } | ||
|
|
||
| if (dd::create_udt_type(thd, *existing_schema, type_name)) { |
There was a problem hiding this comment.
P1: [P1] Enforce privileges before storing the type
This is the point where persistent dictionary state is created, but execute() performs no access check first. Any authenticated user able to issue the statement can therefore create types in schemas where they have no DDL privileges. Check the appropriate schema-level privilege before acquiring locks or modifying the dictionary.
Confidence: 99%
| return true; | ||
| } | ||
|
|
||
| if (trans_commit_stmt(thd) || trans_commit(thd)) { |
There was a problem hiding this comment.
P1: [P1] Binlog CREATE TYPE before committing
The new dictionary object is committed without writing the statement to the binary log. On replicated or point-in-time-recovered systems, the type will exist only on the originating server and later routines referring to it will fail elsewhere. Binlog the DDL before committing and roll back both statement and transaction if logging or commit fails.
Confidence: 98%
| DEFINE_METHOD(int, mysql_udt_registration_imp::unregister_function, | ||
| (mysql_function_descriptor_t * fd)) { | ||
| fprintf(stderr, "mysql_udt_registration_imp::unregister_function() %p\n", fd); | ||
| return 0; |
There was a problem hiding this comment.
P1: [P1] Remove registered callbacks when unloading a component
unregister_function reports success without erasing the hash entry. After UNINSTALL COMPONENT, function lookup can still return descriptors and callbacks located in the unloaded shared library, so the next invocation can jump into unmapped memory; reinstall also retains the stale duplicate. Remove the entry under the lock and reject unloading while its reference count is nonzero.
Confidence: 99%
|
|
||
| for (size_t i = 0; i < count; i++) { | ||
| // FIXME: build proper value | ||
| item = that->get_arg(i); |
There was a problem hiding this comment.
P1: [P1] Validate the actual argument count before indexing arguments
The loop uses the registered descriptor's count rather than the parsed Item_func::arg_count. A call such as complex_number_add() therefore reads beyond the args array and can crash the server. During resolution, verify arity and argument types, and never index beyond the actual argument array.
Confidence: 99%
| UDT_value_in **array = new UDT_value_in *[count]; | ||
| Item *item; | ||
|
|
||
| for (size_t i = 0; i < count; i++) { | ||
| // FIXME: build proper value | ||
| item = that->get_arg(i); | ||
| array[i] = new UDT_value_in(item); |
There was a problem hiding this comment.
P2: [P2] Free per-evaluation argument wrappers
Every function evaluation allocates an array and one UDT_value_in per argument, but none are deleted or attached to a statement memory root. Evaluating a UDT function across many rows therefore grows process heap usage without bound. Use statement-owned allocation or RAII containers whose storage is released after the callback.
Confidence: 99%
| create_type_stmt: | ||
| CREATE TYPE_SYM type_ident AS builtin_type | ||
| { | ||
| $$= NEW_PTN PT_create_type_stmt(@$, $3); |
There was a problem hiding this comment.
P1: [P1] Preserve the declared underlying type
Although the syntax requires AS builtin_type, the parse action discards that value and constructs the command using only the name. The dictionary consequently stores no underlying descriptor, and resolution later hard-codes every type as BLOB(16), so declarations such as AS CHAR(13) have incorrect storage and conversion semantics. Carry the parsed type through the command and persist it in the DD object.
Confidence: 99%
| item = that->get_arg(i); | ||
| array[i] = new UDT_value_in(item); |
There was a problem hiding this comment.
P1: [P1] Enforce an argument-count check before indexing arguments
Calls are never checked against fd->argument_count. A call such as complex_number_add(a) reaches get_arg(1) past the supplied argument array, potentially crashing the server; extra arguments are silently ignored. Validate the Item argument count during itemization/type resolution and return the normal wrong-parameter-count error before building this array.
Confidence: 98%
| DEFINE_METHOD(int, mysql_udt_registration_imp::unregister_function, | ||
| (mysql_function_descriptor_t * fd)) { | ||
| fprintf(stderr, "mysql_udt_registration_imp::unregister_function() %p\n", fd); | ||
| return 0; |
There was a problem hiding this comment.
P1: [P1] Remove function records before unloading their component
unregister_function reports success without removing or disabling the hash entry. After UNINSTALL COMPONENT, later SQL can still resolve the function and invoke record->impl, which points into the unloaded component library, causing a use-after-unload/crash. Remove the entry under the lock and either reject unload while ref_count is nonzero or defer destruction until references are released.
Confidence: 99%
| CREATE TYPE_SYM type_ident AS builtin_type | ||
| { | ||
| $$= NEW_PTN PT_create_type_stmt(@$, $3); |
There was a problem hiding this comment.
P1: [P1] Preserve the declared underlying type
Although the grammar parses AS builtin_type, only $3 (the name) is passed to the command, so the declared type is discarded. The DD object consequently stores only a name and resolve_type_descriptor() maps every UDT to a hard-coded BLOB(16); for example, CREATE TYPE ... AS CHAR(13) does not produce a CHAR(13) variable. Pass the parsed type into the command and persist/restore its complete descriptor.
Confidence: 100%
| #ifdef WITH_EXPERIMENTAL_UDT | ||
| WARN_NOT_IMPLEMENTED(thd, "Sql_cmd_create_type::execute()"); |
There was a problem hiding this comment.
P1: [P1] Check CREATE TYPE privileges before modifying the dictionary
The command locks and stores a dictionary object without any authorization check. Consequently, a user able to issue SQL can create types in schemas where they lack CREATE/appropriate DDL privileges. Perform the schema-level privilege check before acquiring locks or writing the DD object.
Confidence: 94%
| const char *db_name = m_type_ident->db.str; | ||
| const char *type_name = m_type_ident->type.str; |
There was a problem hiding this comment.
P1: [P1] Resolve or reject unqualified CREATE TYPE names
The grammar accepts CREATE TYPE name ..., but that Type_ident has db.str == nullptr; this code immediately treats it as a database name and passes it into schema-lock/string operations, which can dereference null and crash. Resolve an omitted database to the session's current database (with the usual no-database-selected error), or require a qualified name in the grammar.
Confidence: 99%
| for (size_t i = 0; i < count; i++) { | ||
| // FIXME: build proper value | ||
| item = that->get_arg(i); | ||
| array[i] = new UDT_value_in(item); | ||
| } |
There was a problem hiding this comment.
P2: [P2] Free each allocated argument wrapper after evaluation
Every evaluation allocates one UDT_value_in per argument, but cleanup only deletes the pointer array, not the objects; the error path also skips array cleanup. Repeated evaluation across rows therefore leaks heap memory without bound. Use RAII/contiguous storage or delete every wrapper and the array on all exits.
Confidence: 98%
There was a problem hiding this comment.
Codex PR review
Overall: patch is incorrect (99% confidence)
The new UDT path has authorization, type-persistence, input-validation, and component-lifecycle defects that can cause unauthorized dictionary changes, incorrect types, and server crashes.
5 finding(s) were posted inline.
| return true; | ||
| } | ||
|
|
||
| if (dd::create_udt_type(thd, *existing_schema, type_name)) { |
There was a problem hiding this comment.
P1: [P1] Enforce privileges before creating a type
The command reaches the dictionary store without any check_access or dynamic-privilege check. Consequently, a user who can issue the statement can create types in databases where they have no DDL privilege. Check the appropriate schema privilege before acquiring/storing the object, as other DDL commands do.
Confidence: 97%
| #ifdef WITH_EXPERIMENTAL_UDT | ||
| WARN_NOT_IMPLEMENTED(thd, "Sql_cmd_create_type::execute()"); | ||
|
|
||
| const char *db_name = m_type_ident->db.str; |
There was a problem hiding this comment.
P1: [P1] Resolve unqualified type names to the current database
The grammar accepts CREATE TYPE name ..., but an unqualified Type_ident has db.str == nullptr. This value is passed directly into schema locking and dictionary lookup, potentially dereferencing null instead of using the current database (or returning ER_NO_DB_ERROR). Resolve the database before using this pointer.
Confidence: 98%
| CREATE TYPE_SYM type_ident AS builtin_type | ||
| { | ||
| $$= NEW_PTN PT_create_type_stmt(@$, $3); |
There was a problem hiding this comment.
P2: [P2] Preserve the declared underlying type
The parser discards $5, so CREATE TYPE ... AS CHAR(13) and CREATE TYPE ... AS BINARY(16) persist identical name-only objects. Resolution later hard-codes every type to BLOB(16), meaning declarations silently get the wrong storage and conversion semantics. Pass the parsed builtin descriptor through the command and persist it in the DD object.
Confidence: 99%
| for (size_t i = 0; i < count; i++) { | ||
| // FIXME: build proper value | ||
| item = that->get_arg(i); |
There was a problem hiding this comment.
P1: [P1] Validate UDT function argument counts before indexing
count comes from the registered descriptor, while get_arg(i) indexes the arguments supplied by SQL without checking their count. A call such as complex_number_add() therefore reads beyond the Item_func argument array and can crash the server. Validate arity during itemization/type resolution and report a normal wrong-parameter-count error.
Confidence: 98%
| DEFINE_METHOD(int, mysql_udt_registration_imp::unregister_function, | ||
| (mysql_function_descriptor_t * fd)) { | ||
| fprintf(stderr, "mysql_udt_registration_imp::unregister_function() %p\n", fd); | ||
| return 0; |
There was a problem hiding this comment.
P1: [P1] Remove function records before component unload
Unregistration always succeeds without removing the hash entry or honoring its reference count. After the component is unloaded, lookup still returns a record whose descriptor and callback point into the unloaded library, so a later invocation can dereference freed memory or jump to unmapped code. Remove the entry under the lock and reject/defer unload while references remain; item destruction must release acquired references.
Confidence: 99%
There was a problem hiding this comment.
Codex PR review
Overall: patch is incorrect (97% confidence)
The change introduces unauthorized DD writes, unsafe component-unload behavior, and multiple correctness and metadata-visibility defects in the new UDT functionality.
5 finding(s) were posted inline.
| bool Sql_cmd_create_type::execute(THD *thd) { | ||
| bool rc; | ||
|
|
||
| #ifdef WITH_EXPERIMENTAL_UDT |
There was a problem hiding this comment.
P1: [P1] Check schema privileges before creating a type
Execution proceeds directly to locking and storing the DD object without any check_access call. Since dictionary writes do not enforce SQL privileges themselves, an authenticated user can create types in schemas where they lack CREATE privileges. Perform the appropriate schema-level privilege check before acquiring locks or modifying the dictionary.
Confidence: 96%
| DEFINE_METHOD(int, mysql_udt_registration_imp::unregister_function, | ||
| (mysql_function_descriptor_t * fd)) { | ||
| fprintf(stderr, "mysql_udt_registration_imp::unregister_function() %p\n", fd); | ||
| return 0; |
There was a problem hiding this comment.
P1: [P1] Remove function registrations during component unload
unregister_function always succeeds without removing the hash entry, leaving fd and impl pointing into the unloaded component. A later call with the same function name—or reinstalling the component, whose registrations then collide—can invoke unloaded code and crash the server. Remove the entry under the lock and reject unload while its reference count is nonzero; item destruction must also release acquired references.
Confidence: 98%
| create_type_stmt: | ||
| CREATE TYPE_SYM type_ident AS builtin_type | ||
| { | ||
| $$= NEW_PTN PT_create_type_stmt(@$, $3); |
There was a problem hiding this comment.
P2: [P2] Preserve the declared underlying type
The builtin_type matched after AS is discarded when constructing the parse-tree node. Consequently CREATE TYPE ... AS CHAR(13) and ... AS BINARY(16) store identical name-only DD records, and resolution later hard-codes every type as BLOB(16). Pass the parsed descriptor through the command and persist it so declarations retain the requested type semantics.
Confidence: 99%
|
|
||
| Type_ident(const LEX_CSTRING &db_arg, const LEX_CSTRING &type_arg) | ||
| : db(db_arg), type(type_arg) {} | ||
| Type_ident(const LEX_CSTRING &type_arg) : type(type_arg) { db = NULL_CSTR; } |
There was a problem hiding this comment.
P2: [P2] Resolve unqualified type names against the current database
The grammar accepts an unqualified type name, but this constructor leaves db as NULL_CSTR. Both CREATE TYPE execution and declaration resolution immediately use db.str as a schema name, so valid statements such as USE test; CREATE TYPE t AS INT can pass a null schema pointer into locking/dictionary code instead of using test. Resolve an absent qualifier from the THD's current database before those accesses.
Confidence: 96%
| "typ.name" + m_target_def.fs_name_collation()); | ||
|
|
||
| m_target_def.add_from("mysql.types typ"); | ||
| m_target_def.add_from("JOIN mysql.schemata sch ON typ.schema_id=sch.id"); |
There was a problem hiding this comment.
P2: [P2] Filter TYPES by accessible schemas
The new INFORMATION_SCHEMA view joins and exposes every row in mysql.types without an access predicate. Other schema-scoped INFORMATION_SCHEMA views filter inaccessible databases, so users can enumerate type names in schemas for which they have no privileges. Add a CAN_ACCESS_DATABASE(sch.name) predicate (or an equivalent type-specific check).
Confidence: 93%
Proof Of Concepts - MySQL User Defined Types
Contributes to #674
Caution
Proof of concepts code, do not merge, do not use in production.
This code is dirty and to throw away.
The point of this PR is to do prototyping, to identify what needs to be changed in the server, to start a technical discussion.
Quick start
Build
Compile with
-DWITH_EXPERIMENTAL_UDT=ON -DWITH_DEBUG=ONinCMake.Run
Current progress
Declare a UDT and provide an implementation:
Declare a UDT variable, perform operations provided by the component:
Invoke the UDT implementation at runtime:
Cleanup:
Component implementation
Check files under
components/udt_example.